From 4814a84e65d6bb53ada5f4a7e23a1b3befc80ae9 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 26 Jul 2016 15:23:20 -0700 Subject: [PATCH] Addressing review comments --- src/cargo/core/resolver/mod.rs | 4 +- src/cargo/ops/cargo_rustc/fingerprint.rs | 10 +++ src/cargo/sources/registry/index.rs | 87 +++++++++++++----------- src/cargo/sources/registry/local.rs | 15 ++-- src/cargo/sources/registry/mod.rs | 15 ++-- src/cargo/sources/registry/remote.rs | 7 +- tests/directory.rs | 2 +- tests/local-registry.rs | 2 +- tests/lockfile-compat.rs | 4 +- 9 files changed, 89 insertions(+), 57 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 5f91ed735..c774481f1 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -169,7 +169,7 @@ this could be indicative of a few possible situations: but was replaced with one that doesn't * the lock file is corrupt -unable to verify that `{0}` was the same as before in either situation +unable to verify that `{0}` is the same as when the lockfile was generated ", id, id.source_id()) // If the checksums aren't equal, and neither is None, then they @@ -185,7 +185,7 @@ this could be indicative of a few possible errors: * a replacement source in use (e.g. a mirror) returned a different checksum * the source itself may be corrupt in one way or another -unable to verify that `{0}` was the same as before in any situation +unable to verify that `{0}` is the same as when the lockfile was generated ", id); } } diff --git a/src/cargo/ops/cargo_rustc/fingerprint.rs b/src/cargo/ops/cargo_rustc/fingerprint.rs index 91a8b1abb..702deb5f8 100644 --- a/src/cargo/ops/cargo_rustc/fingerprint.rs +++ b/src/cargo/ops/cargo_rustc/fingerprint.rs @@ -57,6 +57,16 @@ pub fn prepare_target<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, let compare = compare_old_fingerprint(&loc, &*fingerprint); log_compare(unit, &compare); + // If our comparison failed (e.g. we're going to trigger a rebuild of this + // crate), then we also ensure the source of the crate passes all + // verification checks before we build it. + // + // The `Source::verify` method is intended to allow sources to execute + // pre-build checks to ensure that the relevant source code is all + // up-to-date and as expected. This is currently used primarily for + // directory sources which will use this hook to perform an integrity check + // on all files in the source to ensure they haven't changed. If they have + // changed then an error is issued. if compare.is_err() { let source_id = unit.pkg.package_id().source_id(); let sources = cx.packages.sources(); diff --git a/src/cargo/sources/registry/index.rs b/src/cargo/sources/registry/index.rs index 4aa59aebc..7be0a309c 100644 --- a/src/cargo/sources/registry/index.rs +++ b/src/cargo/sources/registry/index.rs @@ -1,6 +1,6 @@ use std::collections::HashMap; use std::io::prelude::*; -use std::fs::{File, OpenOptions}; +use std::fs::File; use std::path::Path; use rustc_serialize::json; @@ -8,7 +8,7 @@ use rustc_serialize::json; use core::dependency::{Dependency, DependencyInner, Kind}; use core::{SourceId, Summary, PackageId, Registry}; use sources::registry::{RegistryPackage, RegistryDependency, INDEX_LOCK}; -use util::{CargoResult, ChainError, internal, Filesystem, human, Config}; +use util::{CargoResult, ChainError, internal, Filesystem, Config}; pub struct RegistryIndex<'cfg> { source_id: SourceId, @@ -16,18 +16,21 @@ pub struct RegistryIndex<'cfg> { cache: HashMap>, hashes: HashMap<(String, String), String>, // (name, vers) => cksum config: &'cfg Config, + locked: bool, } impl<'cfg> RegistryIndex<'cfg> { pub fn new(id: &SourceId, path: &Filesystem, - config: &'cfg Config) -> RegistryIndex<'cfg> { + config: &'cfg Config, + locked: bool) -> RegistryIndex<'cfg> { RegistryIndex { source_id: id.clone(), path: path.clone(), cache: HashMap::new(), hashes: HashMap::new(), config: config, + locked: locked, } } @@ -52,34 +55,43 @@ impl<'cfg> RegistryIndex<'cfg> { if self.cache.contains_key(name) { return Ok(self.cache.get(name).unwrap()); } - // If the lock file doesn't already exist then this'll cause *someone* - // to create it. We don't actually care who creates it, and if it's - // already there this should have no effect. - drop(OpenOptions::new() - .write(true) - .create(true) - .open(self.path.join(INDEX_LOCK).into_path_unlocked())); - let lock = self.path.open_ro(Path::new(INDEX_LOCK), - self.config, - "the registry index"); - let file = lock.and_then(|lock| { - let path = lock.path().parent().unwrap(); - let fs_name = name.chars().flat_map(|c| { - c.to_lowercase() - }).collect::(); - - // see module comment for why this is structured the way it is - let path = match fs_name.len() { - 1 => path.join("1").join(&fs_name), - 2 => path.join("2").join(&fs_name), - 3 => path.join("3").join(&fs_name[..1]).join(&fs_name), - _ => path.join(&fs_name[0..2]) - .join(&fs_name[2..4]) - .join(&fs_name), - }; - File::open(&path).map_err(human) - }); - let summaries = match file { + let summaries = try!(self.load_summaries(name)); + let summaries = summaries.into_iter().filter(|summary| { + summary.0.package_id().name() == name + }).collect(); + self.cache.insert(name.to_string(), summaries); + Ok(self.cache.get(name).unwrap()) + } + + fn load_summaries(&mut self, name: &str) -> CargoResult> { + let (path, _lock) = if self.locked { + let lock = self.path.open_ro(Path::new(INDEX_LOCK), + self.config, + "the registry index"); + match lock { + Ok(lock) => { + (lock.path().parent().unwrap().to_path_buf(), Some(lock)) + } + Err(_) => return Ok(Vec::new()), + } + } else { + (self.path.clone().into_path_unlocked(), None) + }; + + let fs_name = name.chars().flat_map(|c| { + c.to_lowercase() + }).collect::(); + + // see module comment for why this is structured the way it is + let path = match fs_name.len() { + 1 => path.join("1").join(&fs_name), + 2 => path.join("2").join(&fs_name), + 3 => path.join("3").join(&fs_name[..1]).join(&fs_name), + _ => path.join(&fs_name[0..2]) + .join(&fs_name[2..4]) + .join(&fs_name), + }; + match File::open(&path) { Ok(mut f) => { let mut contents = String::new(); try!(f.read_to_string(&mut contents)); @@ -87,18 +99,13 @@ impl<'cfg> RegistryIndex<'cfg> { ret = contents.lines().filter(|l| l.trim().len() > 0) .map(|l| self.parse_registry_package(l)) .collect(); - try!(ret.chain_error(|| { + ret.chain_error(|| { internal(format!("failed to parse registry's information \ for: {}", name)) - })) + }) } - Err(..) => Vec::new(), - }; - let summaries = summaries.into_iter().filter(|summary| { - summary.0.package_id().name() == name - }).collect(); - self.cache.insert(name.to_string(), summaries); - Ok(self.cache.get(name).unwrap()) + Err(..) => Ok(Vec::new()), + } } /// Parse a line from the registry's index file into a Summary for a diff --git a/src/cargo/sources/registry/local.rs b/src/cargo/sources/registry/local.rs index 6c2966e53..46387bb68 100644 --- a/src/cargo/sources/registry/local.rs +++ b/src/cargo/sources/registry/local.rs @@ -75,12 +75,17 @@ impl<'cfg> RegistryData for LocalRegistry<'cfg> { // We don't actually need to download anything per-se, we just need to // verify the checksum matches the .crate file itself. - let mut data = Vec::new(); - try!(crate_file.read_to_end(&mut data).chain_error(|| { - human(format!("failed to read `{}`", crate_file.path().display())) - })); let mut state = Sha256::new(); - state.update(&data); + let mut buf = [0; 64 * 1024]; + loop { + let n = try!(crate_file.read(&mut buf).chain_error(|| { + human(format!("failed to read `{}`", crate_file.path().display())) + })); + if n == 0 { + break + } + state.update(&buf[..n]); + } if state.finish().to_hex() != checksum { bail!("failed to verify the checksum of `{}`", pkg) } diff --git a/src/cargo/sources/registry/mod.rs b/src/cargo/sources/registry/mod.rs index 88128f119..3a1babafb 100644 --- a/src/cargo/sources/registry/mod.rs +++ b/src/cargo/sources/registry/mod.rs @@ -181,6 +181,7 @@ pub struct RegistrySource<'cfg> { updated: bool, ops: Box, index: index::RegistryIndex<'cfg>, + index_locked: bool, } #[derive(RustcDecodable)] @@ -240,7 +241,7 @@ impl<'cfg> RegistrySource<'cfg> { config: &'cfg Config) -> RegistrySource<'cfg> { let name = short_name(source_id); let ops = remote::RemoteRegistry::new(source_id, config, &name); - RegistrySource::new(source_id, config, &name, Box::new(ops)) + RegistrySource::new(source_id, config, &name, Box::new(ops), true) } pub fn local(source_id: &SourceId, @@ -248,13 +249,14 @@ impl<'cfg> RegistrySource<'cfg> { config: &'cfg Config) -> RegistrySource<'cfg> { let name = short_name(source_id); let ops = local::LocalRegistry::new(path, config, &name); - RegistrySource::new(source_id, config, &name, Box::new(ops)) + RegistrySource::new(source_id, config, &name, Box::new(ops), false) } fn new(source_id: &SourceId, config: &'cfg Config, name: &str, - ops: Box) -> RegistrySource<'cfg> { + ops: Box, + index_locked: bool) -> RegistrySource<'cfg> { RegistrySource { src_path: config.registry_source_path().join(name), config: config, @@ -262,7 +264,9 @@ impl<'cfg> RegistrySource<'cfg> { updated: false, index: index::RegistryIndex::new(source_id, ops.index_path(), - config), + config, + index_locked), + index_locked: index_locked, ops: ops, } } @@ -306,7 +310,8 @@ impl<'cfg> RegistrySource<'cfg> { let path = self.ops.index_path(); self.index = index::RegistryIndex::new(&self.source_id, path, - self.config); + self.config, + self.index_locked); Ok(()) } } diff --git a/src/cargo/sources/registry/remote.rs b/src/cargo/sources/registry/remote.rs index 65f09a25f..700bd6811 100644 --- a/src/cargo/sources/registry/remote.rs +++ b/src/cargo/sources/registry/remote.rs @@ -53,7 +53,12 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> { } fn update_index(&mut self) -> CargoResult<()> { - // Ensure that this'll actually succeed later on. + // Ensure that we'll actually be able to acquire an HTTP handle later on + // once we start trying to download crates. This will weed out any + // problems with `.cargo/config` configuration related to HTTP. + // + // This way if there's a problem the error gets printed before we even + // hit the index, which may not actually read this configuration. try!(ops::http_handle(self.config)); // Then we actually update the index diff --git a/tests/directory.rs b/tests/directory.rs index aa6808769..9d6dcf147 100644 --- a/tests/directory.rs +++ b/tests/directory.rs @@ -287,7 +287,7 @@ this could be indicative of a few possible errors: * a replacement source in use (e.g. a mirror) returned a different checksum * the source itself may be corrupt in one way or another -unable to verify that `foo v0.1.0` was the same as before in any situation +unable to verify that `foo v0.1.0` is the same as when the lockfile was generated ")); } diff --git a/tests/local-registry.rs b/tests/local-registry.rs index 43bf1fa29..b974f01c4 100644 --- a/tests/local-registry.rs +++ b/tests/local-registry.rs @@ -344,7 +344,7 @@ this could be indicative of a few possible errors: * a replacement source in use (e.g. a mirror) returned a different checksum * the source itself may be corrupt in one way or another -unable to verify that `foo v0.0.1` was the same as before in any situation +unable to verify that `foo v0.0.1` is the same as when the lockfile was generated ")); } diff --git a/tests/lockfile-compat.rs b/tests/lockfile-compat.rs index 64931dc04..5462339fd 100644 --- a/tests/lockfile-compat.rs +++ b/tests/lockfile-compat.rs @@ -155,7 +155,7 @@ this could be indicative of a few possible errors: * a replacement source in use (e.g. a mirror) returned a different checksum * the source itself may be corrupt in one way or another -unable to verify that `foo v0.1.0` was the same as before in any situation +unable to verify that `foo v0.1.0` is the same as when the lockfile was generated ")); } @@ -272,7 +272,7 @@ this could be indicative of a few possible situations: but was replaced with one that doesn't * the lock file is corrupt -unable to verify that `foo v0.1.0 ([..])` was the same as before in either situation +unable to verify that `foo v0.1.0 ([..])` is the same as when the lockfile was generated ")); } -- 2.30.2